Forward kernel telemetry options - #506

Verdict: 1 Medium · 1 Low

Clean, well-tested forwarding of telemetry/runtime identity into the kernel path, and the wrapper-telemetry suppression on useKernel is correct. One medium: getLocaleName uses inverted POSIX locale precedence (checks LANG before LC_ALL/LC_MESSAGES), so overrides are ignored. One low: duplicated env-disable parsing that could drift from DBSQLClient.

Verdict: 1 Medium · 1 Low

Solid, well-tested forwarding change — routing telemetry into kernel openSession and disabling the Node wrapper on the kernel path both look correct and are covered. Two things worth confirming: (1) DBSQLClient always seeds these telemetry config fields, so the isFinite "omit ⇒ kernel default" guards never actually omit — the kernel always inherits Node's defaults; (2) the circuit-breaker threshold/timeout are forwarded without ever setting telemetryCircuitBreakerEnabled. Nit (no anchor needed): getLocaleName/getProcessName are now duplicated between DBSQLClient and KernelAuth with a subtle divergence (the kernel copies apply sanitizeProcessName, the DBSQLClient ones don't) — consider sharing one implementation.

Verdict: 1 Low

Looks good — the telemetry-forwarding logic, env kill-switch unification, and Node-telemetry disabling on the kernel path are correct, well-tested, and all forwarded fields are declared on the napi .d.ts. One low-severity inconsistency: the kernel and Thrift getLocaleName implementations use different env-var precedence, so the same process can report different localeName across backends.

Verdict: 1 Medium

Mechanically sound: field names, guards, and env-parity all check out, and the tests cover the forwarded shape well. One medium concern: buildKernelTelemetryOptions never reads config.telemetryEnabled (declared in its Pick but unused), so kernel telemetry defaults off-by-default while Thrift defaults on — a silent behavioral divergence when a connection switches to useKernel.

Verdict: 2 Low

Looks solid overall — the telemetry-forwarding logic, the telemetryEnabled env/option resolution, and the !useKernel gate that suppresses duplicate Node-wrapper telemetry are all correct and well-tested. Two low-severity notes: a comment in buildKernelTelemetryOptions mislabels telemetryBackoffBaseMs as a user-settable ConnectionOptions knob (it isn't exposed), and the non-positive-value fallbacks silently drop caller input without the log line the sibling env-kill path emits.

Verdict: 1 Medium

Mostly solid — telemetry forwarding, the !useKernel init gate, and the shared isTelemetryDisabledByEnv helper all look correct and are well-tested. One medium concern: because DBSQLClient.getDefaultConfig() always seeds the telemetry tuning knobs, the kernel's "Omitted ⇒ kernel default" path is unreachable in production and the Node defaults silently shadow the kernel's — worth confirming intent and reconciling the comments/tests.

Verdict: 1 Low

Looks good — clean, well-documented telemetry-forwarding change. Verified config field names against ClientConfig/DEFAULT_TELEMETRY_CONFIG, napi target names against native/kernel/index.d.ts, and the > 0 vs >= 0 guards against the documented napi constraints; the telemetryEnabled/env-disable logic and its new tests are correct. One low: the new warnRejected out-of-range branch has no test coverage.

Verdict: 1 Low

Looks good — well-scoped and thoroughly tested. Field names match the napi binding, the isTelemetryDisabledByEnv refactor is behavior-preserving, and the !useKernel telemetry gate plus env kill-switch are correct with solid unit coverage. One Low: the circuit-breaker threshold/timeout are forwarded but telemetryCircuitBreakerEnabled is never set, so those knobs may be inert depending on the kernel default (F1).

Verdict: 1 Low

Looks good — a clean, well-tested telemetry-forwarding change with one trivial low note. I verified every forwarded field against the napi ConnectionOptions contract and every ClientConfig field in the Pick, confirmed the !useKernel telemetry gate + env-disable precedence, and checked the >0/>=0 guard logic against the new tests — all consistent. The only nit is a redundant re-read of DATABRICKS_TELEMETRY_DISABLED in DBSQLClient.connect (raised inline as F1).

Verdict: 1 Low

Looks good — a clean, well-tested change. The telemetry knob forwarding is correct: all ClientConfig knobs are seeded from DEFAULT_TELEMETRY_CONFIG in getDefaultConfig(), copyDefinedTelemetryOptions populates config before backend.connect() reads it, forwarded napi field names match the generated native/kernel/index.d.ts contract, the > 0 / >= 0 guards + warnRejected behave as documented, and the telemetryEnabled env-kill logic is correct across all four cases. One low-severity cross-path behavior note is inline. Nit: the package-lock.json patch bumps (js-yaml, brace-expansion) are unrelated to the telemetry work and appear to be incidental rebase/install churn — worth confirming they're intended for this PR.